Skip to content

feat(wallet): Add split persistence backed by better-sqlite3#8480

Merged
FrederikBolding merged 21 commits intofeat/wallet-libraryfrom
rekm/wallet-sqlite
Apr 22, 2026
Merged

feat(wallet): Add split persistence backed by better-sqlite3#8480
FrederikBolding merged 21 commits intofeat/wallet-libraryfrom
rekm/wallet-sqlite

Conversation

@rekmarks
Copy link
Copy Markdown
Member

@rekmarks rekmarks commented Apr 16, 2026

Summary

  • Add synchronous, per-property SQLite persistence to the wallet package using better-sqlite3
  • Each controller state property with persist: true metadata gets its own row in a kv table (key format: ControllerName.propertyName)
  • Writes happen synchronously within the same call stack as controller.update() via stateChanged event subscriptions, eliminating data loss windows
  • Defaults to :memory: when no database path is provided

Test plan

  • yarn workspace @metamask/wallet exec jest --no-coverage --watchman=false src/persistence/ — 22 unit tests covering KeyValueStore CRUD, loadState grouping, persist filtering, StateDeriver application, patch-based diffing, unsubscribe, and multi-controller scenarios
  • yarn workspace @metamask/wallet exec tsc --noEmit — no new type errors
  • Integration test with file-backed DB: create Wallet with a file path, perform operations, create second Wallet from same path, verify state restoration

🤖 Generated with Claude Code


Note

Medium Risk
Introduces a new persistence layer and changes wallet lifecycle/messenger semantics, plus adds a native dependency (better-sqlite3) that can affect build/install reliability across environments.

Overview
Adds a new persistence subpath export that provides synchronous SQLite-backed state storage via KeyValueStore, plus loadState (reconstruct controller state from Controller.prop keys) and subscribeToChanges (persist only persist-flagged controller properties based on Immer patches and delete on removal).

Updates Wallet construction/typing to accept optional preloaded state, exposes controllerMetadata for initialized controllers, switches messenger namespace to Wallet, and makes destroy() idempotent while publishing a new Wallet:destroyed event after best-effort controller teardown.

Wires in the native better-sqlite3 dependency (with LavaMoat allow-scripts), adds test/CI setup to install required binaries (anvil + better-sqlite3 prebuild) and documents how to rebuild the native addon.

Reviewed by Cursor Bugbot for commit 772c1e6. Bugbot is set up for automated code reviews on this repo. Configure here.

@socket-security
Copy link
Copy Markdown

socket-security Bot commented Apr 16, 2026

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Added@​types/​better-sqlite3@​7.6.131001007180100
Addedbetter-sqlite3@​12.9.010010010091100

View full report

@socket-security

This comment was marked as resolved.

@rekmarks
Copy link
Copy Markdown
Member Author

@SocketSecurity ignore npm/better-sqlite3@11.10.0

Yes, it has native binaries.

@rekmarks
Copy link
Copy Markdown
Member Author

rekmarks commented Apr 16, 2026

@SocketSecurity ignore-all

All alerts are due to better-sqlite3 or its transitives. It is a reputable package that we use elsewhere.

@rekmarks rekmarks force-pushed the rekm/wallet-sqlite branch 6 times, most recently from 8f58b8c to f66328d Compare April 16, 2026 20:30
@rekmarks rekmarks force-pushed the feat/wallet-library branch 2 times, most recently from 623f9af to 3fb60ee Compare April 16, 2026 21:13
@rekmarks rekmarks force-pushed the rekm/wallet-sqlite branch 4 times, most recently from 1d6d1fc to 937ba80 Compare April 16, 2026 21:52
@rekmarks rekmarks marked this pull request as ready for review April 16, 2026 21:59
@rekmarks rekmarks requested a review from a team as a code owner April 16, 2026 21:59
@rekmarks rekmarks force-pushed the rekm/wallet-sqlite branch from 937ba80 to 29c6ac6 Compare April 16, 2026 21:59
Comment thread packages/wallet/src/persistence/persistence.ts
Comment thread packages/wallet/src/Wallet.ts Outdated
Comment thread packages/wallet/src/Wallet.ts Outdated
Comment thread packages/wallet/src/persistence/persistence.ts Outdated
rekmarks and others added 4 commits April 17, 2026 11:50
Persist controller state to SQLite using a per-property key-value
scheme (one row per ControllerName.propertyName). Writes are
synchronous within the same call stack as controller state updates,
using stateChanged events and Immer patches to write only changed,
persist-flagged properties. Defaults to ':memory:' when no database
path is provided.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The build tsconfig is stricter than the dev tsconfig — the union type
of controller instances does not expose the protected `destroy` method.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
rekmarks and others added 8 commits April 17, 2026 11:50
Address review findings: add constructor cleanup on failure, make
destroy() idempotent with Promise.allSettled and finally, handle
remove patches via store.delete(), catch and log handler write
failures, validate degenerate store keys, handle root state
replacement patches, add contextful JSON.parse errors, tighten
loadState return type, extract PersistableController type and
storeKey utility, remove premature KeyValueStore public export,
and add tests for new behaviors.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@rekmarks rekmarks force-pushed the rekm/wallet-sqlite branch from 520a971 to cfad387 Compare April 17, 2026 18:50
Comment thread packages/wallet/src/Wallet.ts Outdated
Make `@metamask/wallet` platform-agnostic by removing the SQLite-backed
`KeyValueStore` from the `Wallet` constructor. `Wallet` now accepts a
flat `WalletOptions` (including an optional `state` payload) and owns no
persistence state. Consumers arrange persistence themselves.

- `Wallet` exposes a `controllerMetadata` getter so persistence can
  operate on metadata alone, without reaching into controller instances.
- A new `Root:walletDestroyed` event is published after controllers tear
  down during `destroy()`. `subscribeToChanges` listens for it and
  self-unsubscribes, eliminating the need for callers to hold an
  unsubscribe handle.
- `subscribeToChanges` now takes a `Record<string, StateMetadataConstraint>`
  instead of the controller-instance map, and specializes its messenger
  type to `RootMessenger<DefaultActions, DefaultEvents>` so subscribing
  to `Root:walletDestroyed` type-checks without suppression.
- `KeyValueStore`, `loadState`, and `subscribeToChanges` are exposed via
  a new `@metamask/wallet/persistence` subpath export; they'll move to
  their own package later.
So that @metamask/wallet/persistence can be moved to its own package
without needing to deep-import from the wallet package.
@rekmarks rekmarks force-pushed the rekm/wallet-sqlite branch from 06b91f1 to f4e8dfa Compare April 17, 2026 22:00
rekmarks and others added 3 commits April 17, 2026 15:07
Renames install-anvil.sh to install-binaries.sh and adds a
better-sqlite3 native addon rebuild step for CI environments where
the prebuilt binding is absent or incompatible with the Node version.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Check for the compiled addon at
`node_modules/better-sqlite3/build/Release/better_sqlite3.node` and skip
`prebuild-install` when it's already present. Avoids unnecessary network
calls on every `test:prepare` run.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 4c16ea9. Configure here.

Comment thread packages/wallet/src/Wallet.ts
const changed = getChangedProperties(patches, persistedProperties);

for (const prop of changed) {
const key = storeKey(controllerName, prop);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The clients do persistence using KV where the key is the controller name. Do we think it is preferable to do at the property level? If so, why?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's faster. SQLite stores a text representation of the JSON values under the hood. Every time we modify a value stored at a given key in the state table, we overwrite the value of that key with JSON.stringify(newValue) (see KeyValueStore.ts). Consequently, by splitting controller state across multiple keys, there is less persistence overhead. There are ways to make this vastly more efficient, but this should be fine for know.

I don't know what the tradeoffs look like for this in the browser or React Native.

persistedProperties: ReadonlySet<string>,
): ReadonlySet<string> {
const changed = new Set<string>();
for (const patch of patches) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally we try to not inspect the patches as it has proven error-prone, it may be fine in this case, but flagging regardless.

Copy link
Copy Markdown
Member Author

@rekmarks rekmarks Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hear that, although in this case we're not deeply inspecting the patches but relying on a couple of invariants:

  • If the patch path length is 0, the entire value has been replaced
  • The first element of the path array is the name of the first modified property

Comment thread packages/wallet/src/Wallet.ts Outdated
Comment thread packages/wallet/src/Wallet.test.ts Outdated
Comment thread packages/wallet/src/Wallet.ts Outdated
@FrederikBolding FrederikBolding merged commit 39b963d into feat/wallet-library Apr 22, 2026
349 checks passed
@FrederikBolding FrederikBolding deleted the rekm/wallet-sqlite branch April 22, 2026 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants